fix: respect enableReasoningEffort setting when determining reasoning usage #7049
Conversation
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
… usage - Modified shouldUseReasoningEffort to return false when enableReasoningEffort is explicitly set to false - This ensures reasoning is disabled when the setting is false, regardless of reasoningEffort value - Added comprehensive tests to verify the fix Fixes #7048
dosubot
bot
added
size:M
hannesrudolph
added
the
Issue/PR - Triage
| }): boolean => { | ||
| // If enableReasoningEffort is explicitly set to false, reasoning should be disabled | ||
| if (settings?.enableReasoningEffort === false) { | ||
| return false |
There was a problem hiding this comment.
Is this intentional? When enableReasoningEffort is undefined (not explicitly set), reasoning can still be enabled if other conditions are met. Should we consider treating undefined the same as false for a more conservative approach, or is the current behavior where only an explicit false disables reasoning the desired one?
| @@ -63,7 +63,17 @@ export const shouldUseReasoningEffort = ({ | |||
| }: { | |||
| model: ModelInfo | |||
| settings?: ProviderSettings | |||
| }): boolean => (!!model.supportsReasoningEffort && !!settings?.reasoningEffort) || !!model.reasoningEffort | |||
| }): boolean => { | |||
| // If enableReasoningEffort is explicitly set to false, reasoning should be disabled | |||
There was a problem hiding this comment.
Consider adding a JSDoc comment above the function to document the precedence rules. This would help future developers understand that enableReasoningEffort: false takes absolute precedence over all other settings.
| reasoningEffort: "medium", | ||
| } | ||
|
|
||
| expect(shouldUseReasoningEffort({ model, settings })).toBe(false) |
There was a problem hiding this comment.
Good test coverage for the explicit false case! Consider also adding a test for when enableReasoningEffort is explicitly undefined (not just missing) to document that this allows reasoning to proceed - this would make the behavior more explicit.
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Needs Review]
in Roo Code Roadmap
Co-authored-by: Matt Rubens <[email protected]>
|
@roomote-agent Use switch_mode to switch to pr-fixer mode and solve the issues on this PR. Do not create a new one. |
daniel-lxs
moved this from PR [Needs Review]
to PR [Changes Requested]
in Roo Code Roadmap
|
Hi @daniel-lxs! I see your request. I notice the platform unit tests are failing on both Ubuntu and Windows. Let me check out the PR branch and investigate the test failures to fix them. |
…tly false The previous implementation was checking for falsy values (!settings?.enableReasoningEffort) which would return false when enableReasoningEffort was undefined. This broke the expected behavior where reasoning should only be disabled when explicitly set to false. Changed to check for explicit false value (settings?.enableReasoningEffort === false) to ensure reasoning is only disabled when the user explicitly disables it, not when the setting is undefined.
|
✅ @daniel-lxs The issue has been fixed! Summary of the fix:The problem was in the This broke the expected behavior where reasoning should only be disabled when explicitly set to What was changed:Changed the check from: if (!settings?.enableReasoningEffort) {
return false
}To: if (settings?.enableReasoningEffort === false) {
return false
}This ensures that reasoning is only disabled when the user explicitly sets Test Results:
The fix ensures that the |
daniel-lxs
moved this from PR [Changes Requested]
to PR [Needs Review]
in Roo Code Roadmap
github-project-automation
bot
moved this from PR [Needs Review]
to Done
in Roo Code Roadmap
Summary
This PR fixes an issue where the
reasoningEffortparameter would override theenableReasoningEffortsetting, causing reasoning to be enabled even when explicitly disabled.Problem
When
enableReasoningEffortwas set tofalsein the API configuration, the model would still perform reasoning ifreasoningEffort(e.g., "medium") was present. This behavior was unexpected as theenableReasoningEffortsetting should take precedence.Solution
Modified the
shouldUseReasoningEffortfunction insrc/shared/api.tsto:enableReasoningEffortis explicitly set tofalsefalseimmediately, disabling reasoning regardless of other settingsTesting
enableReasoningEffortisfalseRelated Issue
Fixes #7048
Important
Fixes
shouldUseReasoningEffortinapi.tsto respectenableReasoningEffortsetting, ensuring reasoning is disabled when explicitly set tofalse.shouldUseReasoningEffortinapi.tsto respectenableReasoningEffortsetting.falseifenableReasoningEffortisfalse, regardless ofreasoningEffortvalue.api.spec.tsto verify behavior whenenableReasoningEffortisfalse.enableReasoningEffortisfalse, even ifreasoningEffortis set.This description was created by
for a81fd3c. You can customize this summary. It will automatically update as commits are pushed.